-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scope of disable_active_migration #3670
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the point below.
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes.
I like @martinthomson's editorial improvements, but even without them, I think we are ready.
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Following on @janaiyengar's insight that disable_active_migration is a property of an address, not a peer, we simply need to say which address the TP applies to.
This PR fixes #3608 by scoping the definition of disable_active_migration to the handshake address only. This means that existing clients remain fully compliant, albeit slightly more conservative than necessary.
While I find @kazuho's argument convincing that a server capable of using SPA is highly unlikely to be unable to handle migration, it would be trivial for a future extension to define an additional transport parameter carrying two bits that disable active migration for those addresses as well.